Skip to content

[Clean-ups] Few cleanups in tb.sv #478

Merged
martin-velay merged 2 commits intolowRISC:mainfrom
KinzaQamar:cleanups
Apr 30, 2026
Merged

[Clean-ups] Few cleanups in tb.sv #478
martin-velay merged 2 commits intolowRISC:mainfrom
KinzaQamar:cleanups

Conversation

@KinzaQamar
Copy link
Copy Markdown
Contributor

@KinzaQamar KinzaQamar commented Apr 28, 2026

No description provided.

@KinzaQamar
[Clean-ups] Removed the unused peripheral clock interface
c23ed05 
Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>
marnovandermaas
marnovandermaas previously approved these changes Apr 29, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me thanks for the cleanup.

@KinzaQamar KinzaQamar marked this pull request as draft April 29, 2026 09:58
@KinzaQamar
Copy link
Copy Markdown
Contributor Author

KinzaQamar commented Apr 29, 2026

I am converting it into draft because the regression result on Xcelium locally looks weird

@marnovandermaas marnovandermaas dismissed their stale review April 29, 2026 10:00

Author mentions the regression on Xcelium is not right.

@KinzaQamar KinzaQamar requested a review from martin-velay April 29, 2026 10:03
@KinzaQamar KinzaQamar marked this pull request as ready for review April 29, 2026 10:40
@KinzaQamar
Copy link
Copy Markdown
Contributor Author

KinzaQamar commented Apr 29, 2026

I am converting it into draft because the regression result on Xcelium locally looks weird

I made a typo in this PR hence causing build failures.

I've ran a regression for all smoke tests which looks like:

|                                      Name                                      |  Passing  |  Total  |  Pass Rate  |  Coverage  |
|:------------------------------------------------------------------------------:|:---------:|:-------:|:-----------:|:----------:|
|                     [UART](scratch/HEAD/reports/uart.html)                     |     9     |    9    |  100.00 %   |     --     |
|            [SPI_DEVICE_2P](scratch/HEAD/reports/spi_device_2p.html)            |     8     |    8    |  100.00 %   |     --     |
|                 [SPI_HOST](scratch/HEAD/reports/spi_host.html)                 |    10     |   10    |  100.00 %   |     --     |
|                 [RV_TIMER](scratch/HEAD/reports/rv_timer.html)                 |     3     |    3    |  100.00 %   |     --     |
|               [PRIM_ALERT](scratch/HEAD/reports/prim_alert.html)               |     2     |    2    |  100.00 %   |     --     |
|                 [PRIM_ESC](scratch/HEAD/reports/prim_esc.html)                 |     1     |    1    |  100.00 %   |     --     |
|                [PRIM_LFSR](scratch/HEAD/reports/prim_lfsr.html)                |     2     |    2    |  100.00 %   |     --     |
|                      [I2C](scratch/HEAD/reports/i2c.html)                      |     8     |    8    |  100.00 %   |     --     |
|                [XBAR_PERI](scratch/HEAD/reports/xbar_peri.html)                |     1     |    1    |  100.00 %   |     --     |
|                     [GPIO](scratch/HEAD/reports/gpio.html)                     |     9     |    9    |  100.00 %   |     --     |
|            [ROM_CTRL_32KB](scratch/HEAD/reports/rom_ctrl_32kb.html)            |     0     |    3    |   0.00 %    |     --     |
| [RV_DM_USE_JTAG_INTERFACE](scratch/HEAD/reports/rv_dm_use_jtag_interface.html) |     0     |    7    |   0.00 %    |     --     |
|            [TOP_MOCHA_SIM](scratch/HEAD/reports/top_mocha_sim.html)            |    18     |   22    |   81.82 %   |     --     |

martin-velay
martin-velay approved these changes Apr 29, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@martin-velay martin-velay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left only nit comments, thanks for this fix and cleanup (and sorry for having introduced it myself)

Comment thread hw/top_chip/dv/tb/tb.sv Outdated
Comment on lines +109 to +110
logic sim_sram_clk;
logic sim_sram_rst;
Copy link
Copy Markdown
Contributor

@martin-velay martin-velay Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nit: do you mind re-ordering these 2 signals to keep the logic of having the clk and rst declared before sim_sram_cpu_req

Comment thread hw/top_chip/dv/tb/tb.sv
logic sim_sram_rst;

assign sim_sram_clk = dut.clkmgr_clocks.clk_main_infra;
assign sim_sram_rst = dut.rstmgr_resets.rst_main_n[rstmgr_pkg::Domain0Sel];
Copy link
Copy Markdown
Contributor

@martin-velay martin-velay Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another very nit: could you jump a line between the assigns and the comment below.
And maybe write a comment above these assign to explain why we need to connect to these internal DUT signals

marnovandermaas
marnovandermaas approved these changes Apr 29, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@marnovandermaas marnovandermaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sorting the build. It would be good to address the minor comments from Martin before merging.

@KinzaQamar
[chip, I2C, dv] Make sram_axi_sim use the right clock and reset
bef3380 
Signed-off-by: Kinza Qamar <kqzaman@lowrisc.org>
@KinzaQamar KinzaQamar requested a review from martin-velay April 30, 2026 15:37
@KinzaQamar
Copy link
Copy Markdown
Contributor Author

KinzaQamar commented Apr 30, 2026

Hi @martin-velay ,

I've addressed your comments. Are you happy with them now?

martin-velay
martin-velay approved these changes Apr 30, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@martin-velay martin-velay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this required fix and cleanups

@martin-velay martin-velay merged commit 28542cc into lowRISC:main Apr 30, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marnovandermaas marnovandermaas marnovandermaas approved these changes

@martin-velay martin-velay martin-velay approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@KinzaQamar @marnovandermaas @martin-velay